Repository navigation
Conversation
…uild A round is 4 builds of 80 entry points. A debug build takes about 0.4 s for one such build, a release build about 0.02 s. The 40 rounds took 58 to 85 s on a debug build, and the test has a timeout of 60 s. A debug build now runs 5 rounds (11 to 17 s). A release build still runs 40. With the entries_mutex snapshot of the route loader removed, the 5-round fixture ends in an ASAN heap-use-after-free in 26 of 26 runs.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Round selection and expected match count test/js/bun/util/filesystem_router.test.ts |
The test imports isDebug, selects 5 rounds when debug and ASAN are both enabled or 40 otherwise, and uses that count for loop iterations and expected matches. |
Possibly related PRs
- oven-sh/bun#46584: Updates the same concurrent
FileSystemRouter.reload()andBun.build()test to select its round count based on debug mode.
Suggested reviewers: alii, jarred-sumner
Priority:
Merge Risk: 
94d8d
-
Merge Risk: ⚪ Minimal · up to
94d8dThe test change preserves coverage for standard builds and reduces only the intended debug-ASAN workload.
Pre-merge checks: 
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title names the main change: the reload() while Bun.build() test now runs 5 rounds on debug builds with ASAN. The PR objectives and the file summary describe the same change, so the title matches … Description check The description clearly explains the problem, the fix, the expected behavior, and verification results. It does not use the exact template heading "What does this PR do?", but the required information… Full details: Title check
-
Explanation
The title names the main change: the reload() while Bun.build() test now runs 5 rounds on debug builds with ASAN. The PR objectives and the file summary describe the same change, so the title matches the changeset. The title is slightly loose because it omits that release builds keep 40 rounds, but it does not mislead.
Full details: Description check
-
Explanation
The description clearly explains the problem, the fix, the expected behavior, and verification results. It does not use the exact template heading "What does this PR do?", but the required information is present and complete.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Status: ready for review. The diff is green. How I reproduced it: CI: both runs ended as failed for one reason that is not in the diff. 2 macOS x64 Pull request: #47241 |
The 5 rounds rest on ASAN, which fails on the first read of a freed listing. A debug build without ASAN has no such check, so it keeps the 40 rounds that every other build runs.
There was a problem hiding this comment.
LGTM — the follow-up commit (72567f0) gates the round cut on isDebug && isASAN, so a debug build without ASAN keeps the base's 40 rounds, which was the one concern raised on the previous version.
What was reviewed:
- The
roundsconstant feeds both the fixture loop bound and the expected stdout (rounds * 50, 50 reload()/match() calls per round), so the exact-output assertion stays in sync for 5 and 40 rounds. isDebugandisASANare the existing harness.ts exports (Bun.version.includes("debug")anddetectASAN()); no new helper was hand-rolled.- The subprocess still drains stdout/stderr/exited via Promise.all and asserts exit code 0 with
signalCode: null, so a UAF crash remains observable; the per-test timeout was not raised.
Extended reasoning...
The change touches only test/js/bun/util/filesystem_router.test.ts, parameterizing the round count of the existing reload()/Bun.build() race test so a debug+ASAN build runs 5 rounds instead of 40 while release and non-ASAN debug builds are unchanged. It touches no security-sensitive surface and no production code. The diff is small and self-contained, the file has no CODEOWNER entry, and the second commit addressed the sole inline finding from the earlier review by narrowing the reduction to the configuration whose ASAN rationale justifies it. No third-party CHANGES_REQUESTED review is outstanding.
robobun
changed the title
reload() while Bun.build() test on a debug buildreload() while Bun.build() test on a debug build with ASAN
Behaviour change: none
Problem
bun bd, ASAN on),reload() while Bun.build() resolves the same directory(test/js/bun/util/filesystem_router.test.ts:1021) fails:this test timed out after 60000ms. Its fixture needs 58 to 85 s there.Bun.build()calls: 0.36 to 0.48 s each on that build, 0.02 s on a release build.Fix
reload()calls.entries_mutexsnapshot in the route loader, the fixture ends inAddressSanitizer: heap-use-after-freein 26 of 26 runs.bun bd test test/js/bun/util/filesystem_router.test.ts, 51 pass, the test takes 11 to 17 s. Main: 50 pass, 1 fail.Background
router.reload()lists a cached directory while the bundler thread reads that directory again in place.MI_DEBUGcommits memory withmprotect. The 12 pool threads wait for the address-space lock (mmap_lock).Notes
All numbers are from one 12-core Linux x64 KVM guest. "Fixture" is the body of the test's
fixture.tswithperformance.now()around the loop. The debug build isbun bd(ASAN on) of main 11c3671.Where the time goes
reload()(4 runs)reload()only, 2,000 callsGOMAXPROCS=4)GOMAXPROCS=2)A release build (367d939) runs the same fixture in 3.1 s.
The 12
Bun Poolthreads have most of the system time (45 of 58 s in one run). The two samples below are from runs of the builds only. A sample of the kernel stacks of the blocked threads (/proc/<pid>/task/<tid>/stack, 9,573 samples) shows what they wait for:mmap_lock(lock_mm_and_find_vma,do_user_addr_fault)mprotect(do_mprotect_pkey)mmap, 2%:madviseThe pool threads are blocked like this for 30% of the run. The guest also reports 287 to 358 s of steal time per run (
/proc/stat). I did not measure how much of the wait the host causes.A sample of the user stacks of the pool threads (13,570 samples that are not asleep) shows who asks for the memory. The ASAN runtime has no frame pointers, so the callers behind it come from a scan of the stack for return addresses:
__asan::Allocator::AllocateImpl), 14% inmprotect, 11% in the first write to a new mimalloc page.Worker::create(src/bundler/ThreadPool.rs:618), which clones theTranspilerand itsBundleOptionsfor each pool thread of each build.rename_symbols_in_chunkand about 8% from the other work for each chunk. The fixture has 80 entry points, so each build has 80 chunks.mimalloc statistics for 161 builds (
Bun.unsafe.mimallocDump()): 30.1 K heaps, 60.0 K pages, 37.7 K commits, 9.5 K purges, 2.4 GiB purged. The commits aremprotectcalls only in a debug build:_mi_prim_decommit(src/prim/unix/prim.cof mimalloc) callsmprotect(PROT_NONE)and asks for a commit later whenMI_DEBUGis not 0.scripts/build/deps/mimalloc.ts:158setsMI_DEBUG=3for a debug build.Settings that do not make 40 rounds fit
Builds only, debug build, one run each. The same run without a setting takes 57.6 to 77.7 s.
MIMALLOC_PURGE_DELAY=-1MIMALLOC_PURGE_DECOMMITS=0ASAN_OPTIONS=allocator_release_to_os_interval_ms=-1The 5 rounds still find the race
I removed the
entries_mutexsnapshot fromRouteLoader::load(src/router/lib.rs) and frombust_dir_cache_recursive(src/runtime/api/filesystem_router.rs), so that both loops read the live map again, and built a debug build. These are the loops that #33056 changed. The fixture then ends like this:heap-use-after-freeThe reports I read show the race of the test: thread T0 reads the map in
RouteLoader::loador inbust_dir_cache_recursive, and theBundlerthread freed it inRealFS::read_directory_with_iteratorunderResolver::load_as_file.Is this a regression?
I did not find one. A debug build of 367d939 (18 September) needs 83.1, 95.3 and 95.4 s for the fixture on the same machine. The runs of the two builds were not interleaved, and one build varies from 58 to 85 s, so these numbers do not show a difference between the builds. I did not build a commit between the two.
What I ran
bun bd test test/js/bun/util/filesystem_router.test.tson this branch, 2 runs, the second on the last commit: 51 pass, 0 fail, 57 and 40 s for the file.this test timed out after 60000ms), 129 s.A debug build without ASAN
It keeps 40 rounds, as on main (Windows, or
--asan=off). The 5 rounds rest on ASAN, and I have no count that is shown to find the race without it. I did not build this configuration, so its time for the test is not measured.Other pull requests
#45617 and #46584 contain the same change of the round count (10 rounds on a debug build) next to other changes.
no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts